python bindings by erip · Pull Request #948 · marian-nmt/marian-dev

erip · 2022-07-18T20:22:59Z

Description

Adds python bindings using pybind11 for high-level bindings.

Added dependencies: pybind11

How to test

python3 -m venv .venv && source .venv/bin/activate
pip install pybind11
cd python
python -m pip install .

Checklist

I have tested the code manually
I have run regression tests
I have read and followed CONTRIBUTING.md
I have updated CHANGELOG.md

…or a batch to improve ease-of-use.

…n-dev into python

…nd split unnecessarily. (#950)

…ons.

…n-dev into feature/python-bindings

…mt/marian-dev into feature/python-bindings

erip · 2022-07-31T19:39:39Z

 include_directories(3rd_party)
 include_directories(3rd_party/SQLiteCpp/include)
-include_directories(3rd_party/sentencepiece)
+# include_directories(3rd_party/sentencepiece)


Is this meant to be commented out? I am not sure why spm_encode isn't found in CI on OS X, but I suspect this is the culprit.

I don't think so. I've updated GitHub workflows recently, which may fix issues on macOS. I will rebase this branch with the current master and re-run builds.

Thanks, @snukky! It still seems like there's something fishy happening on the spm front...

snukky · 2022-11-02T11:18:45Z

Is this still WIP as the title suggests or is it ready for reviews as it's not marked as a draft?

erip · 2022-11-02T12:37:31Z

This is ready for review. There are some conflicts, but I won't be able to address them until early next week. The CI failure seems to be related to disk on the OS X build. We'll need to follow a similar recipe as Windows to clear intermediate build directories and this should be OK.

erip · 2022-11-12T14:15:01Z

It seems like the OS X error here is due to 3pp zstr here. I'm not sure why OS X is falling into this branch since L30 should cover the OS X case... Is there a missing cmake flag somewhere?

snukky

Many thanks Elijah for this PR again. As it was forever since this PR was open, I'm happy to take over, rebase with the current master and introduce the changes I'm proposing. I had mostly comments on improving documentation of the code.

I think other remaining tasks related to this code would be 1) fixing MacOS compilation, and 2) adding some regression tests on actual models, but we can take care of them in separate PRs.

Thanks!

snukky · 2023-06-19T12:40:23Z

        ./marian --version
        ./marian-decoder --version
        ./marian-scorer --version
-        ./spm_encode --version


What was the motivation for this to get removed?

snukky · 2023-06-19T12:41:10Z

          -DCOMPILE_CPU=${{ matrix.cpu }} \
          -DCOMPILE_CUDA=${{ matrix.gpu }} \
          -DCOMPILE_EXAMPLES=${{ matrix.examples }} \
+          -DUSE_TCMALLOC=OFF \


Python bindings doesn't work with TCMalloc? Why? Would be great to have it fixed or at least commented/documented.

snukky · 2023-06-19T12:41:30Z

-        ./marian-scorer --version
-        ./marian-server --version
-        ./spm_encode --version


As above, why do we need to remove these checks?

snukky · 2023-06-19T12:43:13Z

+        cd ..
+        rd /s /q build


Is this for debugging? I would suggest adding a short comment above explaining what it does and why it is done.

snukky · 2023-06-19T12:43:49Z

@@ -1,4 +1,3 @@
-# Config files from CMake


I think this comment is still valid, isn't it?

snukky · 2023-06-19T13:11:16Z

@@ -0,0 +1,27 @@
+import sys


Consider adding a short comment at the top of this file what this script does and a usage example.

snukky · 2023-06-19T13:12:22Z

 public:
  virtual ~TranslateService() {}

+  TranslateService(const std::string& cliString)


Nit: consider adding a short docstring explaining what it is used for.

snukky · 2023-06-19T13:14:30Z

@@ -0,0 +1,27 @@
+import sys


Is any of those files used for regression tests? Could it easily be?

snukky · 2023-06-19T13:16:00Z

  }

-  std::string run(const std::string& input) override {
+  std::vector<std::string> run(const std::vector<std::string>& inputs, const std::string& yamlOverridesStr="") override {


I think the second parameter deserves a short description in a docstring.

snukky · 2023-06-19T13:17:04Z

+      return utils::split(translations, "\n", /*keepEmpty=*/true);
+  }
+
+  std::string run(const std::string& input, const std::string& yamlOverridesStr="") override {


I think this function also could receive a docstring.

…n-dev into feature/python-bindings

emjotde · 2023-08-21T22:15:42Z

@snukky let's go ahead with this. We can also recruit @mjpost and @rjai to help.

emjotde · 2023-08-21T22:32:18Z

Oh, and @thammegowda of course

emjotde · 2023-08-21T22:37:56Z

A first thing that I see is that this should use the python package of SentencePiece from src/3rdparty

* This code is same as [public github repo tg/pybind-new branch](#1013). Git histories seems slightly different between public and private repo so we are seeing a lot of commits * This builds on top of work by Elijah #948

erip and others added 11 commits July 18, 2022 14:54

add initial code.

a6d6073

fix typo.

1f6a883

fix a few reference errors.

696b21a

add stupid constructor

2e3cb4f

latest changes

0d648bb

add pybind11 submodule

b656975

working CMake compilation + installation

11cfa21

add test script

351eb35

disable TCMALLOC in module

f03c369

add config parsing

820bef1

rename script

e52f7d1

erip commented Jul 19, 2022

View reviewed changes

Comment thread CMakeLists.txt

erip and others added 6 commits July 19, 2022 08:27

overload run to accept invidual (potentially newline-joined) strings …

e5cc7c4

…or a batch to improve ease-of-use.

add python builds to gitignore.

90919ef

fix overload resolution errors in build.

bc48bf8

batching test

3bff35e

Merge branch 'feature/python-bindings' of github.com:marian-nmt/maria…

52f1d6f

…n-dev into python

add python builds to gitignore.

b1eedce

erip force-pushed the feature/python-bindings branch from 52f1d6f to b1eedce Compare July 19, 2022 17:54

erip and others added 4 commits July 19, 2022 14:10

add coarse benchmarking to test inference. (#949)

efdc157

add vector translate. TODO: revisit the underlying code to not join a…

703643a

…nd split unnecessarily. (#950)

merge

4fd32cc

fix bad merge.

792630c

alvations marked this pull request as ready for review July 19, 2022 21:27

erip and others added 6 commits July 19, 2022 15:09

refactor build to skbuild instead of raw setuptools and cmake extensi…

5fb41b9

…ons.

update README build instructions.

b8b8522

parallel model reading

ba28c92

Merge branch 'feature/python-bindings' of github.com:marian-nmt/maria…

1073489

…n-dev into feature/python-bindings

testing before cmake

37f8a07

remove the pymarian from this branch, working on the fork of this first

3ad4ac7

erip and others added 9 commits July 24, 2022 10:33

revert packaging issue.

e0ba10f

reduce number of parallel workers to mirror normal cmake build.

c48227f

remove __init__.py

b349d8b

Merge branch 'feature/python-bindings' of https://github.com/marian-n…

50d7332

…mt/marian-dev into feature/python-bindings

change to LF

0e63804

add proper tests.

5b91cf7

revert to modern OS X

15e87fe

clean up build directory to save disk.

b1c871a

do not work in a dead directory.

2ccc541

erip commented Jul 31, 2022

View reviewed changes

snukky added 2 commits September 5, 2022 16:58

Merge branch 'master' into feature/python-bindings

4199d37

Include 3rd_party/sentencepiece again

3599f09

jerinphilip reviewed Sep 11, 2022

View reviewed changes

Comment thread src/3rd_party/CMakeLists.txt

erip added 3 commits September 13, 2022 09:11

remove spm_encode from test since we are statically linking

b398263

clean up build dir and pick correct working dir when building pymarian.

3d8c64b

clean up build dir and pick correct working dir when building pymarian.

08c4ef0

erip force-pushed the feature/python-bindings branch from 3d8c64b to 08c4ef0 Compare September 21, 2022 11:44

use unix make instead of ninja due to bug encountered during dev.

87ff8ad

erip changed the title ~~[WIP] python bindings~~ python bindings Nov 2, 2022

Merge branch 'master' into feature/python-bindings

4661fa9

snukky requested changes Jun 19, 2023

View reviewed changes

emjotde added 2 commits August 21, 2023 15:07

Merge branch 'master' into feature/python-bindings

2c5863c

Merge branch 'feature/python-bindings' of github.com:marian-nmt/maria…

4b9588f

…n-dev into feature/python-bindings

Conversation

erip commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to test

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

snukky commented Nov 2, 2022

Uh oh!

erip commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erip commented Nov 12, 2022

Uh oh!

snukky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emjotde commented Aug 21, 2023

Uh oh!

emjotde commented Aug 21, 2023

Uh oh!

emjotde commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

erip commented Jul 18, 2022 •

edited

Loading

erip commented Nov 2, 2022 •

edited

Loading